fix(#4718): stop running pre-code/pre-fix scripts twice per run#175
fix(#4718): stop running pre-code/pre-fix scripts twice per run#175waynesun09 wants to merge 1 commit into
Conversation
Ported from the fullsend-ai/fullsend#4762 discussion — harness configs and pre/post scripts live here now, so this is where the actual fix belongs. reusable-code.yml's inline "Validate inputs" step and pre-fix.sh's equivalent both duplicate the harness pre_script that `fullsend run` already executes right before sandbox creation. The inline call can't just be deleted for code: its skipped= output gates four downstream workflow steps (GCP setup, bot identity, agent env, the agent run itself) that all run before `fullsend run` is invoked. - scripts/pre-code.sh: gate the existing-human-PR search/label/comment behind a new CODE_SKIP_EXISTING_PR_CHECK env var. Unset (used by the inline workflow step) preserves today's behavior exactly. - harness/code.yaml: set CODE_SKIP_EXISTING_PR_CHECK=true in forge.github.runner_env, so only the harness pre_script invocation skips the already-performed check. - scripts/pre-fix.sh: gate the pre-commit tool auto-install behind a new FIX_SKIP_TOOL_INSTALL env var, set by reusable-fix.yml's inline step (fullsend-ai/fullsend side) so it can still fail fast on bad input/iteration cap without repeating the tool install. - scripts/pre-code-test.sh, scripts/pre-fix-test.sh: new test files — neither pre-script had test coverage in this repo. Cover input validation, the existing behaviors, and both new flags. Companion fix in fullsend-ai/fullsend wires GH_TOKEN into the inline "Validate inputs" step (needed for the check to actually run there) and re-adds reusable-fix.yml's inline step with FIX_SKIP_TOOL_INSTALL. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 1:49 PM UTC · Completed 1:59 PM UTC |
PR Summary by QodoDeduplicate pre-code/pre-fix pre-scripts via skip flags + add coverage
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Unsanitized values in ::notice::
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if [[ -n "${HUMAN_PR_LINES}" ]]; then | ||
| # Parse the first PR for the notice. | ||
| FIRST_PR_NUM="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f1)" | ||
| FIRST_PR_AUTHOR="$(echo "${HUMAN_PR_LINES}" | head -1 | cut -f2)" | ||
|
|
||
| echo "::notice::Found existing human PR #${FIRST_PR_NUM} by @${FIRST_PR_AUTHOR}" | ||
|
|
There was a problem hiding this comment.
1. Unsanitized values in ::notice:: 📜 Skill insight ⛨ Security
The PR emits GitHub Actions workflow commands (e.g., ::notice:: and ::warning::) that interpolate variables (FIRST_PR_NUM, FIRST_PR_AUTHOR, RESOLVE_SCRIPT, INSTALL_SCRIPT) without sanitizing them for :: sequences, encoded newlines, ANSI/control characters. This can enable workflow-command injection if any of those values contain unsafe characters (for example from unexpected CLI output).
Agent Prompt
## Issue description
`scripts/pre-code.sh` and `scripts/pre-fix.sh` emit GitHub Actions workflow commands (`::notice::` / `::warning::`) that interpolate variables (`FIRST_PR_NUM`, `FIRST_PR_AUTHOR`, `RESOLVE_SCRIPT`, `INSTALL_SCRIPT`) without sanitizing each value individually.
## Issue Context
GitHub Actions workflow commands are security-sensitive; any interpolated value must be sanitized to prevent workflow-command injection via `::`, `%0A/%0D` (encoded newlines), ANSI escape sequences, or other control characters. Ensure every variable inserted into workflow-command output is sanitized before being echoed.
## Fix Focus Areas
- scripts/pre-code.sh[103-109]
- scripts/pre-fix.sh[145-152]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Skips the already-performed existing-PR check (fullsend-ai/fullsend#4718) | ||
| CODE_SKIP_EXISTING_PR_CHECK: "true" |
There was a problem hiding this comment.
2. Protected paths modified in pr 📜 Skill insight § Compliance
This PR modifies protected governance/infrastructure paths (harness/ and scripts/), which must receive human review and must not be auto-approved. Even though the PR links a justification (issue #4718), a protected-path finding is still required.
Agent Prompt
## Issue description
Protected governance/infrastructure paths were modified, which requires explicit human review and must not be auto-approved.
## Issue Context
Protected paths include `harness/` and `scripts/`.
## Fix Focus Areas
- harness/code.yaml[65-66]
- scripts/pre-code.sh[71-141]
- scripts/pre-fix.sh[116-181]
- scripts/pre-code-test.sh[1-559]
- scripts/pre-fix-test.sh[1-150]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Patch placeholders with actual paths (avoid sed on source files, | ||
| # but this is a generated mock — not repo source code). | ||
| local escaped_log="${gh_log//\//\\/}" | ||
| local escaped_out="${TMPDIR//\//\\/}\/pr-list-output.txt" | ||
| perl -pi -e "s/LOGFILE_PLACEHOLDER/${escaped_log}/g" "${mock_bin}/gh" | ||
| perl -pi -e "s/OUTPUT_PLACEHOLDER/${escaped_out}/g" "${mock_bin}/gh" | ||
|
|
There was a problem hiding this comment.
3. Undeclared perl test dependency 🐞 Bug ☼ Reliability
scripts/pre-code-test.sh requires perl to patch placeholders in the generated mock gh binary, but the script-test workflow does not declare/install perl. This can make make script-test fail on minimal/self-hosted runners (or if the base runner image changes) even though the code under test is fine.
Agent Prompt
### Issue description
`scripts/pre-code-test.sh` currently uses `perl -pi` to patch placeholder strings in a generated mock `gh` script. This introduces an undeclared runtime dependency for `make script-test`, which can break on environments where Perl is not present.
### Issue Context
The repo’s CI job for script tests installs Python + some pip deps, but does not explicitly install Perl. While GitHub-hosted Ubuntu images typically include Perl today, this is an implicit dependency and reduces portability/reproducibility.
### Fix Focus Areas
- scripts/pre-code-test.sh[71-77]
- .github/workflows/script-test.yml[23-35]
### Suggested fixes (pick one)
1) **Avoid Perl**: generate the mock `gh` script with variable interpolation (or use a tool already required/installed, e.g. `python3 -c`), removing the `perl -pi` calls entirely.
2) **Declare dependency**: explicitly install perl in `.github/workflows/script-test.yml` (and document it as a prerequisite for local runs), so the test suite has deterministic dependencies.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
LGTM, let's see what agents have to say.
ReviewVerdict: Approve This PR correctly deduplicates What was reviewed
Observations
Labels: PR modifies pre-code/pre-fix scripts and code agent harness config |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| echo "skipped=false" >> "${GITHUB_OUTPUT:-/dev/null}" | ||
| exit 0 | ||
| fi | ||
| if [[ "${CODE_SKIP_EXISTING_PR_CHECK:-false}" == "true" ]]; then |
There was a problem hiding this comment.
[low] edge-case
CODE_SKIP_EXISTING_PR_CHECK uses exact string comparison == "true" (lowercase only). Non-canonical truthy values ("TRUE", "1", "yes") will not activate the skip, causing the check to run a second time (the original double-run behavior). The failure mode is safe but could cause debugging confusion.
Suggested fix: Consider documenting in the inline comment that only the exact lowercase string "true" is accepted. Same applies to FIX_SKIP_TOOL_INSTALL in pre-fix.sh.
fullsend-ai-review
Bot
added
requires-manual-review
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
I changed my mind, I do think we need to make this false by now, which is the current behavior. Then we set it to true in the workflows. This being true by default means that nothing happens on local executions, which we may want. I think having this as true would make scenario confusing.
Records the pattern introduced in #5013/fullsend-ai/agents#175 so the next agent that needs to gate expensive workflow setup behind a fast pre-check (per rh-hemartin's request for consistency across agents, one: an {AGENT}_SKIP_{THING} env var, set on whichever of the two pre-script call sites should skip the redundant/expensive part. Distinguishes this from ADR 0049's agent-configuration env vars — same {AGENT}_{SETTING} syntax, but an internal invocation-context signal rather than a user-facing behavioral knob, so it belongs in script/harness comments, not docs/agents/<agent>.md. Scopes the decision to agents that actually need workflow-level gating (code, fix today) rather than mandating it retroactively for agents that don't currently duplicate pre-script execution. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
2 participants
Summary
pre-code.shandpre-fix.sheach run twice per agent invocation — once inline in fullsend-ai/fullsend's reusable workflow, once again as the harnesspre_scriptinsidefullsend run. This is the harness/scripts half of the fix (harness configs and pre/post scripts moved to this repo out of fullsend-ai/fullsend'sinternal/scaffold/fullsend-repo/).Related Issue
Fixes fullsend-ai/fullsend#4718
Companion PR
fullsend-ai/fullsend#5013 — wires
GH_TOKENinto the inlineValidate inputsstep (reusable-code.yml) and re-adds a lightweight inline step withFIX_SKIP_TOOL_INSTALL=true(reusable-fix.yml). This repo's changes have no effect without that companion PR, and vice versa — they were designed together.Design
For
code: the inline workflow step'sskipped=output gates four downstream workflow steps (GCP setup, bot identity, agent env, the agent run itself) that all run beforefullsend runis invoked — so the inline call can't simply be deleted. Instead,CODE_SKIP_EXISTING_PR_CHECKgates the expensive/side-effecting part (GH API search,pr-openlabel, issue comment) so it only runs once, on the inline call; the harness invocation skips straight to the tool-install section it's actually there for.For
fix: there's no output-gating dependency, but the inline call is still needed to fail fast on bad input/an exceeded iteration cap before GCP/agent-env setup runs.FIX_SKIP_TOOL_INSTALLlets the inline call skip only the pre-commit tool auto-install (which the harness invocation still needs, for the post-script's pre-commit run).Changes
scripts/pre-code.sh— wrap the existing-human-PR check behindCODE_SKIP_EXISTING_PR_CHECK. Unset (default) preserves current behavior exactly.harness/code.yaml— setCODE_SKIP_EXISTING_PR_CHECK: "true"inforge.github.runner_env, so only the harnesspre_scriptinvocation skips the already-performed check.scripts/pre-fix.sh— wrap the pre-commit tool auto-install behindFIX_SKIP_TOOL_INSTALL.scripts/pre-code-test.sh,scripts/pre-fix-test.sh— new files. Neither pre-script had test coverage in this repo; added coverage for existing behavior plus both new flags (26 + 12 cases).Makefile— wire both new test files intoscript-test.Testing
pre-commit run(staged changes) passes — yaml, shellcheck, secrets, etc.make script-testpasses for both new/modified files (38/38); one pre-existing, unrelated failure inpost-scribe-test.shconfirmed present on unmodifiedmaintoo.